Closed Bug 658245 Opened 13 years ago Closed 7 years ago

Open bookmarks in a new tab configurable

Categories

(Firefox :: Bookmarks & History, enhancement, P3)

Product:
Firefox
Component:
Bookmarks & History
Version:
4.0 Branch
Platform:
x86
Windows 7
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: aquilax, Assigned: hectorz, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files)

Bug 658245 - Part 1: pref to open bookmark in a new tab.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 658245 - Part 2: add a browser mochitest for loadBookmarksInTabs.
59 bytes, text/x-review-board-request
mak
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

It would be nice if it would be possible to choose to open bookmarks always in a new tab, perhaps in the about:config. For people used to open everything in a new tab, opening a bookmark result in loosing the actual tab which is really frustrating.

I know that there are x ways to open bookmarks in a new tab:
- middle mouse button (I have a laptop)
- ctrl-click (I have always to push two buttons and use two hands to open a bookmark?)
- context-menu (Thanks, right-click, find the open in new tab, left click, it's a good training for the touch pad, something easier?)
- tabmixplus (nice I have to install an add-on with 100 unneeded features to just open bookmarks in a new tab?)

I'm not alone, just google firefox open bookmarks in a new tab and you will see that is a common request.


Reproducible: Always

Steps to Reproduce:
1. open the bookmarks menu
2. click any bookmarks
3.

Actual Results:  
The bookmark opens in the current tab

Expected Results:  
The bookmark opens in a new tab
Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110519 Firefox/6.0a1

My bookmarks always open in a new tab, both in Windows and Linux with new clean profiles, when clicking with the middle mouse button.
Version: unspecified → 4.0 Branch
@Thomas Ahlblom


I know that there are x ways to open bookmarks in a new tab:
- middle mouse button ------->>>>>>(I have a laptop)<<<<<<<--------
....

In case that you haven't seen it I mark it a little bit more, if you find the middle "mouse" button on a touch pad you are a genius, otherwise ....
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Open Bookmarks in New Tab (37,759 users) and Tab Mix Plus (651,358 users) (which offers similar functionality) will no longer work with WebExtensions.

I tried to make a WebExtension with this functionality using webRequest, blocking onBeforeRequest and when originUrl is undefined, searching in bookmarks and opening a new tab. This works most of the time but there are numerous edge cases where it fails and behaves undesirably (navigating through history, opening multiple foreground bookmarks rapidly, extensions opening/redirecting to bookmarked sites, AMO bookmarks, about:page bookmarks). Using webRequest also requires access to all websites and there is a delay between the bookmark starting to load in the current tab and the new tab being created.

The middle-mouse button is not available or cumbersome on a lot of input devices and Ctrl + click is similarly clumsy. Opening a bookmark in a pinned tab and "Open All in Tabs" already create new tabs by default. There is a strong argument for preserving the current tab when opening bookmarks (Bug 216346) and it allows opening multiple bookmarks simultaneously which is a significant productivity booster.

In light of this please consider reopening this bug.
Flags: needinfo?(mak77)
yes, we could probably add a pref since the code is all in PlacesUIUtils. I see the add-on I suggested previously has a good number of users, so it sounds like something people may want.

It would be great if someone from the community may work on this, the code to look into is
openNodeWithEvent
I suppose we could get the whereToOpenLink value, if it's "current" check the pref and whether the node is a bookmark, and change it to "tab" or "tabshifted".
We should probably have defineLazyPreferenceGetter at the top of the module for both "browser.tabs.loadBookmarksInBackground" and a new "browser.tabs.loadBookmarksInTabs"

It would also need a mochitest-browser test in browser/components/places/tests/browser/ it may just simulate a click on the bookmarks sidebar, other tests in the same folder are doing the same so they should be partially copyable.

I'm unlikely to be able to work on this, so it would be great if someone from the community may look into it. I can review the suggested patches.
Mentor: mak77
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(mak77)
Priority: -- → P3
Resolution: WONTFIX → ---
Whiteboard: [good first bug][lang=js]
Hey I would like to work on this, I am new to the codebase and this would be my first bug, so could you guide me where to start?
Flags: needinfo?(mak77)
This document should have most of the information:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #7)
> This document should have most of the information:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

I am familiar with the basic information i want to ask that where would i found the code to work on this bug.
You can use http://searchfox.org/mozilla-central/source to search the codebase. Comment 5 points out some of the things to look into (openNodeWithEvent in PlacesUIUTils) and it also points out where a test may be added.
Blocks: 1383669
(In reply to Shivang Agarwal from comment #6)
> Hey I would like to work on this, I am new to the codebase and this would be
> my first bug, so could you guide me where to start?

Hello Shivang, I'm gonna steal this bug since we really want this change in Fx 57 and it's already a month since you last commented. Please let me know if you're still interested in working on this and will upload a patch in the next week or so, thanks.
Assignee: nobody → bzhao
Comment on attachment 8899801 [details]
Bug 658245 - Part 1: pref to open bookmark in a new tab.

https://reviewboard.mozilla.org/r/171130/#review177872

::: browser/components/places/PlacesUIUtils.jsm:1025
(Diff revision 1)
>    openNodeWithEvent:
>    function PUIU_openNodeWithEvent(aNode, aEvent) {
>      let window = aEvent.target.ownerGlobal;
> -    this._openNodeIn(aNode, window.whereToOpenLink(aEvent, false, true), window);
> +
> +    let where = window.whereToOpenLink(aEvent, false, true);
> +    let isBookmark = PlacesUtils.nodeIsBookmark(aNode);

just inline this in the if condition

::: browser/components/places/PlacesUIUtils.jsm:1027
(Diff revision 1)
>      let window = aEvent.target.ownerGlobal;
> -    this._openNodeIn(aNode, window.whereToOpenLink(aEvent, false, true), window);
> +
> +    let where = window.whereToOpenLink(aEvent, false, true);
> +    let isBookmark = PlacesUtils.nodeIsBookmark(aNode);
> +    if (where == "current" && isBookmark &&
> +        this.loadBookmarksInTabs && !aNode.uri.startsWith("javascript:")) {

nit: as a micro-optimization, probably checking this.loadBookmarksInTabs is cheaper than PlacesUtils.nodeIsBookmark, so it may be checked first.
Attachment #8899801 - Flags: review?(mak77) → review+
Comment on attachment 8899802 [details]
Bug 658245 - Part 2: add a browser mochitest for loadBookmarksInTabs.

https://reviewboard.mozilla.org/r/171132/#review177876

I know you are basing off an existing test, unfortunately a lot of the tests are quite old. Now we can write better and more stable tests using promises and shared utils, so I'm suggesting some changes to get a better test.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:10
(Diff revision 1)
> +const BASE_URL = "http://mochi.test:8888/browser/browser/components/places/tests/browser/";
> +const PREF_LOAD_BOOKMARKS_IN_BACKGROUND = "browser.tabs.loadBookmarksInBackground";
> +const PREF_LOAD_BOOKMARKS_IN_TABS = "browser.tabs.loadBookmarksInTabs";
> +const TEST_PAGES = [
> +  BASE_URL + "bookmark_dummy_1.html",
> +  BASE_URL + "bookmark_dummy_2.html"

fwiw, we could use about:mozilla and about:robots here, and avoid the support files.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:19
(Diff revision 1)
> +var gCurrentTest = null;
> +var gItemIds = [];
> +var gTests = [];
> +
> +// Listener for TabOpen and tabs progress.
> +var gTabsListener = {

instead of this custom method, now you could use BrowserUtils.waitForNewTab or BrowserUtils.browserLoaded for the case where a tab is not opened.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:117
(Diff revision 1)
> +  desc: "Open bookmarks by click, with loadBookmarksInTabs set to true.",
> +  tabsToOpen: 2,
> +  urisToLoad: TEST_PAGES,
> +
> +  run() {
> +    gPrefService.setBoolPref(PREF_LOAD_BOOKMARKS_IN_TABS, true);

Should ensure through a registerCleanupFunction that the pref is reset even if the test should timeout.
Or use await SpecialPowers.pushPrefEnv(...

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:171
(Diff revision 1)
> +
> +var toolbar = document.getElementById("PersonalToolbar");
> +var wasCollapsed = toolbar.collapsed;
> +
> +function test() {
> +  waitForExplicitFinish();

could you please instead use the more modern add_task test runner, and avoid the runNextTest() cruft?

In the end the test will look more readable and manageable.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:176
(Diff revision 1)
> +  waitForExplicitFinish();
> +
> +  // Add a bookmark to the personal toolbar.
> +  var bs = PlacesUtils.bookmarks;
> +  TEST_PAGES.forEach((testUrl, index) => {
> +    gItemIds.push(bs.insertBookmark(bs.toolbarFolder,

should use the new async bookmarking API, that is await PlacesUtils.bookmarks.insert({ ... })

The things here using itemId should be changed to use the guid instead (each node has a bookmarkGuid property, so instead of children[i]._placesNode.itemId you'd use children[i]._placesNode.bookmarkGuid)

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:188
(Diff revision 1)
> +  gBrowser.tabContainer.addEventListener("TabOpen", gTabsListener);
> +  gBrowser.addTabsProgressListener(gTabsListener);
> +
> +  // Uncollapse the personal toolbar if needed.
> +  if (wasCollapsed) {
> +    promiseSetToolbarVisibility(toolbar, true).then(runNextTest);

should await and use registerCleanupFunction to restore the previous state.
Attachment #8899802 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [::mak] from comment #14)
> Comment on attachment 8899802 [details]
> Bug 658245 - Part 2: add a browser mochitest for loadBookmarksInTabs.
> 
> https://reviewboard.mozilla.org/r/171132/#review177876
> 
> I know you are basing off an existing test, unfortunately a lot of the tests
> are quite old. Now we can write better and more stable tests using promises
> and shared utils, so I'm suggesting some changes to get a better test.

I'm still not familiar with writing tests, thanks for the suggestions, I'll update the patch next week.
Comment on attachment 8899802 [details]
Bug 658245 - Part 2: add a browser mochitest for loadBookmarksInTabs.

https://reviewboard.mozilla.org/r/171132/#review179416

It looks much better, please do a Try run with a few (5-6) retriggers of the browser-chrome test where this test runs, just to ensure there isn't any new intermittent introduced. Thank you!

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:13
(Diff revision 2)
> +var gBookmarkElements = [];
> +
> +function getToolbarNodeForItemGuid(aItemGuid) {
> +  var children = document.getElementById("PlacesToolbarItems").childNodes;
> +  var node = null;
> +  for (var i = 0; i < children.length; i++) {

afaik, you can use
for (let child of chidren) {
  ... return child;
}
return null;

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:25
(Diff revision 2)
> +}
> +
> +function waitForLoad(browser, expectedUrl, action) {
> +  return BrowserTestUtils.browserLoaded(browser).then(url => {
> +    Assert.equal(url,
> +      expectedUrl, `${action} opens it in current browser`);

you can pass a third argument to browserLoaded (Second argument can be false) that is the href of the page you are waiting a load for, so you don't need to add a further Assert.equal here.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:34
(Diff revision 2)
> +}
> +
> +function waitForNewTab(url, action, inBackground) {
> +  return BrowserTestUtils.waitForNewTab(gBrowser).then(tab => {
> +    Assert.equal(gBrowser.getBrowserForTab(tab).currentURI.spec,
> +      url, `${action} opens it in a new tab`);

second argument to waitForNewTab can be the url to wait for, so you can remove the Assert.equal

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:78
(Diff revision 2)
> +    if (wasCollapsed) {
> +      await promiseSetToolbarVisibility(toolbar, false);
> +    }
> +
> +    await Promise.all(bookmarks.map(bookmark => {
> +      return PlacesUtils.bookmarks.remove(bookmark.guid);

just ...remove(bookmark); should do, no reason to manually extract the guid prop.

::: browser/components/places/tests/browser/browser_click_bookmarks_on_toolbar.js:144
(Diff revision 2)
> +    button: 0
> +  });
> +
> +  await promise;
> +
> +  await SpecialPowers.popPrefEnv();

nit: too many newlines make the code a bit sparse
Attachment #8899802 - Flags: review?(mak77) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=291d5e2f377443b17826ef504dcbc5acca78734f

bc1 triggered multiple times on Windows and macOS per:

(In reply to Marco Bonardo [::mak] from comment #18)
> 
> It looks much better, please do a Try run with a few (5-6) retriggers of the
> browser-chrome test where this test runs, just to ensure there isn't any new
> intermittent introduced. Thank you!
Keywords: checkin-needed
Keywords: checkin-needed
I triggered autoland.
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/4fdf0a1290c1
Part 1: pref to open bookmark in a new tab. r=mak
https://hg.mozilla.org/integration/autoland/rev/7c4820c4af1f
Part 2: add a browser mochitest for loadBookmarksInTabs. r=mak
(In reply to Marco Bonardo [::mak] from comment #23)
> I triggered autoland.

Thank you!
https://hg.mozilla.org/mozilla-central/rev/4fdf0a1290c1
https://hg.mozilla.org/mozilla-central/rev/7c4820c4af1f
Status: REOPENED → RESOLVED
Closed: 13 years ago → 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Hi everyone. 
The provided change is much appreciated.
It makes working with tabs so much easier.
However I have suggestion to make this feature even more polished.
By default (browser.tabs.loadBookmarksInTabs:false) left-click opens bookmark in current tab and middle-click opens tab in new tab.
After setting browser.tabs.loadBookmarksInTabs:true left-click starts to open bookmark in new tab. 
But the middle mouse-click still works same way.
Maybe it is possible to set middle-mouse click to open bookmark in current tab when browser.tabs.loadBookmarksInTabs:true.
In this case browser.tabs.loadBookmarksInTabs will act as a toggle to switch left-click and middle-click functionality.
Please, file your enh request in its own bug. Whether it could be done or not, depends on the complexity and future maintenance costs.
Depends on: 1397372
Depends on: 1406744
Depends on: 1410945
I've come here since receiving the update to FF57.

I made the changes to about:config and when I access a bookmark through the bookmarks list (fro example Ctrl+Shift+B) then the bookmarked URL opens as expected in a new tab. Great.

However, I a bookmarks toolbar which sits below the main toolbar with a number of regularly used bookmarks. These include RSS feeds. The links to individual items in the RSS feeds do not act like other URLs and open directly in the current tab rather than in a new tab as expected.

Steps to reproduce with appropriate setting 'true' in about:config

Include RSS feed in Bookmark Toolbar
Select desired RSS feed item.
It opens in current tab not in a new tab as expected.
please file a new bug.
(In reply to Marco Bonardo [::mak] from comment #30)
> please file a new bug.

Thanks I've added to bug 1410945
Depends on: 1426929
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: